Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(datastore): implement ModelIndex #1366

Merged
merged 4 commits into from
Mar 3, 2022

Conversation

HuiSF
Copy link
Member

@HuiSF HuiSF commented Feb 16, 2022

Issue #, if available:

#1426

Description of changes:

Currently, codegen generated Dart models don't contain model index information. This PR is adding relevant interface and implementation to support upcoming changes in codegen to provide index information in model schema.

  • Implement ModelIndex and add it to ModelSchemaDefinition
  • Update test models
  • Update unit tests

Note: this PR doesn't include Flutter native implementation to convert serialize model indexes into native objects. This implementation will be added in a separate PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@HuiSF HuiSF requested a review from a team as a code owner February 16, 2022 19:20
@HuiSF HuiSF changed the base branch from main to feat/custom-pk February 21, 2022 23:21
@@ -152,6 +155,10 @@ class Blog extends Model {
modelSchemaDefinition.name = "Blog";
modelSchemaDefinition.pluralName = "Blogs";

modelSchemaDefinition.indexes = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a difference between an empty array and an array with the id field marked with @primaryKey? Wondering if we need to always include the primary key field or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing :)
This is simply following GraphQL transformer + codegen convention of how to describe an index, e.g. @primaryKey

@model with normal id as the default primary key

{ name: 'key', arguments: { fields: [ 'id' ] } }

@model with modelID annotated by @primaryKey

{ name: 'key', arguments: { fields: [ 'modelID' ] } }

@model with productId annotated by @primaryKey + sortKeyFields

{
  name: 'key',
  arguments: { fields: [ 'productId', 'productName', 'warehouseId', 'region' ] }
}

And @index

@model has postId annotated with @index + sortKeyFields

{
  name: 'byPost',
  arguments: { fields: [ 'postId', 'content' ] }
}

@@ -69,34 +61,16 @@ class Person extends Model {
}
}

TemporalDateTime? get createdAt {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to createdAt/updatedAt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a custom type, should not contain model meta fields. I think the original generated code was outdated...

}
}
/*
* Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this is registering as a change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this PR converts this file from CRLF to LF mode (The first commit of this PR).
If you view the second commit it shows the actual changes on this file.

@haverchuck haverchuck self-requested a review March 2, 2022 19:53
Copy link
Member

@haverchuck haverchuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one question about a diffing issue, but otherwise LGTM

Copy link
Member

@Jordan-Nelson Jordan-Nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few nit comments but lgtm

String toString() => 'ModelIndex(name: $name, fields: $fields)';

@override
bool operator ==(Object other) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we start using the AWSEquatable mixin instead of implementing these everywhere? It is currently in amplify_flutter, so you would have to move it if you want to use it. I don't think you have to use it for this PR, but I want to discuss if we want to start using it moving forward.

@dnys1 I think you added AWSEquatable. Any reason we can't move that to amplify_core and start using it throughout all packages?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, wasn't aware of this mixin. Can we also use it with generated models? @dnys1

@codecov-commenter
Copy link

Codecov Report

Merging #1366 (743ade3) into feat/custom-pk (a04c1c1) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

@@                Coverage Diff                 @@
##           feat/custom-pk    #1366      +/-   ##
==================================================
- Coverage           47.64%   47.51%   -0.13%     
==================================================
  Files                 245      246       +1     
  Lines               10224    10251      +27     
==================================================
  Hits                 4871     4871              
- Misses               5353     5380      +27     
Flag Coverage Δ
android-unit-tests ∅ <ø> (∅)
flutter-unit-tests 36.47% <0.00%> (-0.12%) ⬇️
ios-unit-tests 95.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...amplify_core/lib/src/types/models/model_index.dart 0.00% <0.00%> (ø)
...mplify_core/lib/src/types/models/model_schema.dart 0.00% <0.00%> (ø)
.../lib/src/types/models/model_schema_definition.dart 0.00% <0.00%> (ø)

@HuiSF HuiSF merged commit ae81d1f into aws-amplify:feat/custom-pk Mar 3, 2022
@HuiSF HuiSF deleted the feat/datastore-model-index branch March 3, 2022 20:35
HuiSF added a commit to HuiSF/amplify-flutter that referenced this pull request Apr 4, 2022
* Convert CRLF to LF

* feat(datastore): implement ModelIndex

* chore(datastore): update tests to cover model indexes

* Improve doc
HuiSF added a commit to HuiSF/amplify-flutter that referenced this pull request Apr 4, 2022
* Convert CRLF to LF

* feat(datastore): implement ModelIndex

* chore(datastore): update tests to cover model indexes

* Improve doc
HuiSF added a commit that referenced this pull request Apr 6, 2022
* Convert CRLF to LF

* feat(datastore): implement ModelIndex

* chore(datastore): update tests to cover model indexes

* Improve doc
HuiSF added a commit to HuiSF/amplify-flutter that referenced this pull request May 20, 2022
* Convert CRLF to LF

* feat(datastore): implement ModelIndex

* chore(datastore): update tests to cover model indexes

* Improve doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants